fix: broken normalisePath/normalisePathWin on windows #1134
Conversation
|
Any chance you could add a windows test GitHub action ? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1134 +/- ##
==========================================
+ Coverage 82.72% 83.14% +0.42%
==========================================
Files 162 164 +2
Lines 9080 7688 -1392
==========================================
- Hits 7511 6392 -1119
+ Misses 1319 1042 -277
- Partials 250 254 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@jptosso I never implemented it, but I try. I hacked it together for testing in another repo and it seems to work. If I apply all my patches and skip the FTW test everything runs through. The runner change just required me to add it to the job matrix. https://github.com/jabdr/corazawin/actions/runs/10540932309/job/29206137841 What do you have in mind? Should it run on every PR/push like the current job or as a cron? |
|
It's a tough question, but if it's not much of a problem, it should be part of our standard workflow. Years ago it was decided that Windows compatibility wasn't going to be a priority because there were no Windows users. But if we gain some traction with Windows, it could be interesting. Also, we would expect all tests to pass for Windows, even for the win. It can be a long-term project though. Cc @jcchavezs @fzipi |
|
I don't mind having the windows tests around, I think they will add value. There should also be a test for this change targeting Windows specific code, right? (it will increase coverage also) |
|
From reading https://github.com/owasp-modsecurity/ModSecurity/wiki/Reference-Manual-(v2.x)-Transformation-Functions#normalizepathwin, isn't exactly what you are doing? Maybe what needs to be done is only run the test to normalize path on *nix and then write tests for |
|
@fzipi Not sure, but I think you misunderstand the documentation. It is expected that the output uses forward slash / for both functions. normalisePathWin (also) supports backward slashes in the input. This works on linux, as on linux filepath.Clean uses forward slashes for the output. The problem is that the OUTPUT is wrong on windows (for both functions), as filepath.Clean uses backward slash \ for it's output on windows. filepath.Clean supports forward slash for input on all platforms. The test for windows and linux are the same (both functions already have tests with many examples), but it requires an actual windows platform to run it, because filepath.Separator is a hardcoded const in go. |
|
Ah, maybe that was the case. Thanks for taking the time to explain it 🙂. |
| @@ -13,10 +14,15 @@ func normalisePath(data string) (string, bool, error) { | |||
| return data, false, nil | |||
| } | |||
| clean := filepath.Clean(data) | |||
There was a problem hiding this comment.
same approach as in #1135 (review) for a Clean function.
There was a problem hiding this comment.
As said in the other issue, the compiler optimizes the if away. Using build tags and seperate files would make this far more complicated.
|
Please rebase your PR on top of |
|
@jabdr Can you follow up on this one please? |
Currently the normalisePath transformation tests fail on windows. This is due to the behaviour difference of filepath.Clean on different platforms. filepath.Clean uses filepath.Separator for it's output, but a / seperated path is expected.
Furthermore, now there is also a check for \ as path directory ending.
This fixes the normalisePath and normalisePathWin transformation tests on windows.